chore: remove refs to AllowUpdateToExpired/Frozen client booleans#1768
chore: remove refs to AllowUpdateToExpired/Frozen client booleans#1768charleenfei merged 10 commits intomainfrom
Conversation
|
question @colin-axner -- should the proto fields also be removed? currently it's API breaking (still need to finish updating the CHANGELOG) but updated the protos would make it also state machine breaking (right?) should we just do this in one go? |
Codecov Report
@@ Coverage Diff @@
## main #1768 +/- ##
==========================================
- Coverage 80.04% 79.88% -0.17%
==========================================
Files 166 166
Lines 12421 12436 +15
==========================================
- Hits 9943 9935 -8
- Misses 2013 2033 +20
- Partials 465 468 +3
|
charleenfei
requested review from
AdityaSripal,
chatton,
colin-axner,
crodriguezvega,
damiannolan and
seantking
as code owners
…c-go into charly/remove_ref_allow_update
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
LGTM! Just one small comment on the changelog entry 🥇
CHANGELOG.md
Outdated
| * (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. | ||
| * (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of non-deterministic writes to application state. | ||
| * (core/04-channel)[\#1636](https://github.com/cosmos/ibc-go/pull/1636) Removing `SplitChannelVersion` and `MergeChannelVersions` functions since they are not used. | ||
| * (light-clients/tendermint)[\#1768](https://github.com/cosmos/ibc-go/pull/1768/files) Removed `AllowUpdateAfter...` booleans as they are deprecated (see ADR026) |
There was a problem hiding this comment.
[nit] maybe we should explicitly list the booleans to make it more clear what exactly was removed?
There was a problem hiding this comment.
makes sense, i will update!
| newClientState := NewClientState( | ||
| tmUpgradeClient.ChainId, cs.TrustLevel, cs.TrustingPeriod, tmUpgradeClient.UnbondingPeriod, | ||
| cs.MaxClockDrift, tmUpgradeClient.LatestHeight, tmUpgradeClient.ProofSpecs, tmUpgradeClient.UpgradePath, | ||
| cs.AllowUpdateAfterExpiry, cs.AllowUpdateAfterMisbehaviour, |
There was a problem hiding this comment.
I guess there's a sideaffect here. These parameters will be set to false, false upon upgrades (even if the previous values were true, true). Shouldn't cause an issue, but is worth being aware of
There was a problem hiding this comment.
i can add an inline doc for this :)
There was a problem hiding this comment.
added a note in upgrade docs about this
|
It probably makes sense to include this in v6 instead of v5 (ie wait for the release branch to be created), just in case there's any issues for users with the changing of |
|
|
|
4 participants
Description
closes #1237
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes